Feat: Support drag file to open #33
Conversation
|
This pull request is deployed automatically with Vercel. samuwrite |
src/app/app.tsx
Outdated
| const dropRef = useFileDrop({ file }); | ||
|
|
||
| return ( | ||
| <div className={s.app}> | ||
| <div className={s.app} ref={dropRef}> |
There was a problem hiding this comment.
This is good, but know that there's an alternative where we should define the ref outside and pass it as param. In those cases, it would be simpler if an element needs to have several refs (not this case though)
src/app/state/file-drop.ts
Outdated
| export const useFileDrop = (params: Params): React.RefObject<HTMLDivElement> => { | ||
| const dropRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| const handleDragOver = (e: DragEvent) => e.preventDefault(); |
There was a problem hiding this comment.
This function can be defined outside of the hook. Would be a little bit better if we don't re-create it unnecessarily
src/app/state/file-drop.ts
Outdated
|
|
||
| useEffect(() => { | ||
| const { current } = dropRef; | ||
| if (!current) return; |
There was a problem hiding this comment.
This should throw Error instead of simply return. If we forget to attach the ref, the drag and drop would not working, which should be an error in user's perpective
src/app/state/file-drop.ts
Outdated
| const { items } = e.dataTransfer; | ||
| if (1 < items.length) throw Error("Only one file can be upload at a time"); | ||
| if (Array.from(items).some((item) => item.kind !== "file")) | ||
| throw Error("Support upload single file only"); |
There was a problem hiding this comment.
This error message and the one at line 16 sound very similar. One should be about the number of files, while the other is about the "folder" type not supported
src/app/state/file-drop.ts
Outdated
| if (1 < items.length) throw Error("Only one file can be upload at a time"); | ||
| if (Array.from(items).some((item) => item.kind !== "file")) | ||
| throw Error("Support upload single file only"); | ||
| if (items && items.length) { |
There was a problem hiding this comment.
Hm why do we still need these checks? Shouldn't it be checked already above?
src/app/state/file-drop.ts
Outdated
| params.file.setHandle(file as FileSystemFileHandle); | ||
| params.file.setDirty(false); |
There was a problem hiding this comment.
I'm starting to see this is quite repetitive, and it's easy to forget. I think there should be a method in the FileState that simply calls these 2:
params.file.setFile(file); // set handle AND set dirty to false
src/app/state/file-drop.ts
Outdated
| } catch (e) { | ||
| throw Error(e); | ||
| } |
There was a problem hiding this comment.
If we only catch to throw then we don't need the try at all. The error would still be throw up if there is no catch
src/app/state/file-drop.ts
Outdated
| current.removeEventListener("dragover", handleDragOver); | ||
| current.removeEventListener("drop", handleDrop); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Why there is no dependency array!
Resolve #28